-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tar: Improve unseekable stream handling #84279
Tar: Improve unseekable stream handling #84279
Conversation
Tagging subscribers to this area: @dotnet/area-system-formats-tar Issue DetailsFixes #76690 Addresses the issue reported in dotnet/sdk-container-builds#192 (comment) by @rainersigwald Before this change, it wasn't possible to write into an archive a file entry that contained an unseekable data stream. The undelying cause was that we were calling The way I fixed this was by separating the code that writes an entry into two possible paths:
|
I'll remove the new file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it LGTM, but it would be nice if we could reduce the code duplication before merging.
Thank you @carlossanlop !
src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.WriteEntryAsync.Tests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectory.Stream.Tests.cs
Outdated
Show resolved
Hide resolved
Adjust existing unseekable stream tests to verify correct for all formats and with multiple tar entries.
… stream into an unseekable archive stream.
…in a separate PR.
… Add extra debug asserts.
3a94f35
to
796d542
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please apply my suggestions before merging.
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHeader.Write.cs
Outdated
Show resolved
Hide resolved
Failures are unrelated. |
Fixes #76690
Addresses the issue reported in dotnet/sdk-container-builds#192 (comment) by @rainersigwald
Before this change, it wasn't possible to write into an archive a file entry that contained an unseekable data stream. The undelying cause was that we were calling
Length
to try to get the total number of bytes of the data stream, but it is unsupported when the stream'sCanSeek
isfalse
.The way I fixed this was by separating the code that writes an entry into two possible paths:
One path for entries with seekable data streams, which remains mostly unmodified.
A new path for entries with unseekable streams, which changes the order in which we collect the entry information into the buffer: